Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added working otsu and mean thresholding #15

Merged
merged 7 commits into from
Aug 31, 2019
Merged

Conversation

jmetz
Copy link
Contributor

@jmetz jmetz commented Aug 29, 2019

The Otsu and Mean threshold implementations mentioned in #13

Copy link
Member

@xd009642 xd009642 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small comments, apart from these looks good to me 👍

src/processing/threshold.rs Outdated Show resolved Hide resolved
src/processing/threshold.rs Outdated Show resolved Hide resolved
src/processing/threshold.rs Outdated Show resolved Hide resolved
}


fn apply_threshold<T>(data: &Array3<T>, threshold: f64) -> Array3<bool>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have threshold as f64 and not type T?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general thresholding algorithms sometimes generate thresholds of a different type to the input (consider for example the mean threshold on ints, which at least from a theory perspective should be a float); if casting back to T is used, then we would need to decide on a strategy (round or floor?), which would likely depend also on whether the threshold value is used for > or >= comparison when being applied to generate the binary result.

Am certainly open to adjusting this to T if we can decide on a consistent strategy that is also in-line with how this is done in other frameworks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That all makes sense, I'm happy with keeping it as f64

@xd009642
Copy link
Member

Oh another comment, if a thresholding algorithm is only assumed to or designed to work with a single channel image you can always check the number of channels and return a channel dimension mismatch error. I do that in the canny module as canny doesn't make sense with >1 channels

* Removed commented-out code
* Removed debug println
* Corrected typo in the comments
@xd009642
Copy link
Member

Awesome all looks good to me, if you just update the changelog to mention the thresholding module and algorithms in the unreleased additions I can merge this in 👍

@xd009642 xd009642 merged commit fdaf03a into rust-cv:develop Aug 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants